Async send prefactor #4044
Async send prefactor #4044
Conversation
This method was rustfmt'd in the previous commit, here we clean up that formatting.
This method will be edited in upcoming commits, and the codebase policy is to at least consider removing rustfmt::skips when touching a method.
This method will be edited in upcoming commits, and the codebase policy is to at least consider removing rustfmt::skips when touching a method.
|
👋 Thanks for assigning @joostjager as a reviewer! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4044 +/- ##
==========================================
+ Coverage 88.73% 88.79% +0.06%
==========================================
Files 176 176
Lines 129042 129310 +268
Branches 129042 129310 +268
==========================================
+ Hits 114501 114824 +323
+ Misses 11939 11891 -48
+ Partials 2602 2595 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
valentinewallace
force-pushed
the
2025-09-async-send-prefactor
branch
4 times, most recently
from
dfbf3ae to
408001c
Compare
| }); | ||
|
|
||
| if let PendingHTLCRouting::Forward { short_channel_id, .. } = payment.forward_info.routing { | ||
| let htlc_source = HTLCSource::PreviousHopData(payment.htlc_previous_hop_data()); |
There was a problem hiding this comment.
Phantom secret was set to None always, where as now it's taken from forward_info. No problem? Also at the last change in this file.
There was a problem hiding this comment.
It should always be None here I believe regardless, since intercept payments and phantom payments both lean on special scids I don't think it would be possible to have a payment be both intercept and phantom.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
| $secp_ctx, | ||
| &Scalar::from_be_bytes(hop_pk_blinding_factor).unwrap(), | ||
| )? | ||
| pk.mul_tweak($secp_ctx, &Scalar::from_be_bytes(hop_pk_blinding_factor).unwrap()) |
There was a problem hiding this comment.
Do you know what the original reason was for making an exception here and not panicking for blinded paths specifically? Is there any way in which input causing this could be provided from the outside?
There was a problem hiding this comment.
No I don't believe so, I wrote the code originally and I think it just made the code look better and there was no reason not to handle the error at the time.
In upcoming commits, we will be adding several more conversions from PendingAddHTLCInfo into HTLCPreviousHopData. This conversion gets repeated all over the ChannelManager already, so lay some groundwork by DRYing it up.
Without this DRYing, we would be repeating the same code to instantiate the PendingAddHTLCInfo several more times in this method, in upcoming commits.
The LDK codebase in general is comfortable panicking if the entropy source provided to it is dysfunctional. Up until now we made an exception for blinded path creation, where we would handle an error that could occur on mul_tweak that could only occur if the session_priv provided was not actually random. In comparable cases in onion_utils, we would panic instead. In upcoming commits, we will be including blinded paths in outbound revoke_and_ack messages as part of implementing async payments, where it is difficult to handle failing back an HTLC if blinded path creation fails. Thus we now have an incentive to make the blinded path creation methods infallible, so do so here.
valentinewallace
force-pushed
the
2025-09-async-send-prefactor
branch
from
408001c to
11f3476
Compare
|
✅ Added second reviewer: @tankyleo |
|
Apologies @elnosh, I missed your LGTM. |
| htlc_id: prev_htlc_id, | ||
| incoming_packet_shared_secret: forward_info | ||
| .incoming_shared_secret, | ||
| phantom_shared_secret: None, |
There was a problem hiding this comment.
Joost mentioned this above @valentinewallace can you confirm we can set this to Some in the PendingHTLCRouting::Receive case ?
There was a problem hiding this comment.
Discussed offline, this part is unreachable in the PendingHTLCRouting::Receive case. In that case, scid == 0 and fake_scid::is_valid_intercept called before this line will return false for any scid equal to 0.
Some cleanups in preparation for implementing sending payments as an often-offline sender to an often-offline recipient.
cc #2298